Action register by Iximiel · Pull Request #935 · plumed/plumed2

Iximiel · 2023-05-12T12:56:12Z

Description

added to ActionRegister.h a new template class ActionRegistration that works like the, PLUMED_REGISTER_ACTION without the need of the preprocessor.
changed PLUMED_REGISTER_ACTION in ActionRegister.h: now is a shortcut to a declaration: #define PLUMED_REGISTER_ACTION(classname,directive) ActionRegistration<classname> directive##Registerer(#directive); instead of a whole class definition+declaration
changed PLUMED_REGISTER_ACTION in the sources with: for a in */; do sed -r 's%(PLUMED_REGISTER_ACTION$.*,.*)\"([A-Z_0-9]*)\"($)%\1\2\3%g' "$a"/*cpp -i; done the string constant is automatically stringified by the "#" operator in the preprocessor
- it is a sed away to be completely exchanged by the explicit template tool
- even if, I think that keeping the macro will keep the operation less verbose

In the case of multiple definitions, like XDistances and XYTorsion, the compiler will only declare one specialized ActionRegistration, but with multiple differently named instances without errors, for example

ActionRegistration<XDistances> xDistancesRegister("XDISTANCES");
ActionRegistration<XDistances> yDistancesRegister("YDISTANCES");
ActionRegistration<XDistances> zDistancesRegister("ZDISTANCES");

Target release

I would like my code to appear in the C++17 release (it uses a inline constexpr variable).

Type of contribution

changes to code or doc authored by PLUMED developers, or additions of code in the core or within the default modules
changes to a module not authored by you
new module contribution or edit of a module authored by you

Copyright

[C] I agree to transfer the copyright of the code I have written to the PLUMED developers or to the author of the code I am modifying.

the module I added or modified contains a COPYRIGHT file with the correct license information. Code should be released under an open source license. I also used the command cd src && ./header.sh mymodulename in order to make sure the headers of the module are correct.

Tests

I added a new regtest or modified an existing regtest to validate my changes.
I verified that all regtests are passed successfully on GitHub Actions.

GiovanniBussi · 2023-05-12T13:22:24Z

src/core/ActionRegister.h

 #include <memory>

-namespace PLMD {
+namespace PLMD { //{


@Iximiel why this additional open brace after the comment?

I'm not a vim user, but is what I understood to do from this

That's only for macros. Here the braces are already available, vim can match them.

GiovanniBussi · 2023-05-12T13:31:56Z

@Iximiel I do not understand the removal of the quotes. Is it to allow multiple names for multiple registrations?

Notice that this has two negative effects:

It requires an extensive change on the code base, including contributed modules, with possible issues when merging or rebasing from one version to another
To me the syntax of the registration is actually less intuitive. If it's a string, one expect to pass a string.

I think using the line number to distinguish the two names (as it is now) is a cleaner solution.

GiovanniBussi · 2023-05-12T13:33:34Z

src/core/ActionRegister.h

+//and the next template will be template<ActionType ActionType>
+
+template<typename ActionClass>
+class plumedRegisterer{ 


We usually use capital letters for classes (PlumedRegisterer)

i corrected that in the fourth commit, is that not up?

Maybe I was reviewing an older commit sorry for this

Iximiel · 2023-05-12T13:59:49Z

Yes, I wanted to allow multiple names by using the wanted directive as a variable name and use the # to get the cstring for the directive

I suspected that removing the quotes was a too heavy change

If you prefer, I'll reverting that and substituting:
#define PLUMED_REGISTER_ACTION(classname,directive) ActionRegistration<classname> directive##Registerer(#directive);

with
#define PLUMED_REGISTER_ACTION(classname,directive) ActionRegistration<classname> classname##__LINE__##Registerer(directive);
as I said is just a sed (or a git rebase) away
so there will be no virtual changes in the other files

GiovanniBussi · 2023-05-12T14:08:46Z

Yes I think it's cleaner thanks!!!

codecov-commenter · 2023-05-12T14:09:10Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.05 ⚠️

Comparison is base (03032e2) 85.78% compared to head (25df5e7) 85.73%.

❗ Current head 25df5e7 differs from pull request most recent head 7016c11. Consider uploading reports for the commit 7016c11 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##            c++17     #935      +/-   ##
==========================================
- Coverage   85.78%   85.73%   -0.05%     
==========================================
  Files         600      600              
  Lines       54724    54437     -287     
==========================================
- Hits        46945    46673     -272     
+ Misses       7779     7764      -15

Impacted Files	Coverage Δ
src/core/ActionRegister.h	`100.00% <100.00%> (ø)`

... and 272 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

GiovanniBussi · 2023-05-12T15:51:34Z

src/core/ActionRegister.h


 #define PLUMED_CONCATENATE_DIRECT(s1, s2) s1##s2
 #define PLUMED_CONCATENATE(s1, s2) PLUMED_CONCATENATE_DIRECT(s1, s2)
-#define PLUMED_UNIQUENAME(str) PLUMED_CONCATENATE(str, __LINE__)


You eliminated an indirection. I honestly do not remember anymore why it was there. Are you sure this is ok?

I think it's correct. You are just using PLUMED_CONCATENATE below..

GiovanniBussi · 2023-05-12T15:53:42Z

.gitignore

 /config.*
 /autom4*
 /stamp-h
+/build


maybe this can be a standalone commit?

removed from this PR

Iximiel added 4 commits May 12, 2023 10:28

PLUMED_REGISTER_ACTION overhaul, plus sed applied to the actions

841cba1

Merge remote-tracking branch 'upstream/c++17'

550c70b

astyled and polished the class

91b5280

small aestetic changes

5c5ba2f

GiovanniBussi reviewed May 12, 2023

View reviewed changes

Iximiel added 5 commits May 12, 2023 16:25

starting to revert to quotes

1a69fd0

readded the quotes

3a14e0f

removed some spaces

2b9345b

removed spaces

f6909bf

reverted the documentation

e4213a7

GiovanniBussi reviewed May 12, 2023

View reviewed changes

moved the definitions outside the PLMD namespace

7016c11

GiovanniBussi merged commit 3e60f26 into plumed:c++17 May 19, 2023

Conversation

Iximiel commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Target release

Type of contribution

Copyright

Tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GiovanniBussi commented May 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Iximiel commented May 12, 2023

Uh oh!

GiovanniBussi commented May 12, 2023

Uh oh!

codecov-commenter commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Iximiel commented May 12, 2023 •

edited

Loading

codecov-commenter commented May 12, 2023 •

edited

Loading